ITE-11: Support attribute constraints in layouts #50
Conversation
|
CEL seems like a solid choice for this. It's simple enough to reason about easily while still being powerful enough to do the vast majority of the types of policies surrounding predicates that we care about. Cole and I have been working with some more complex policies that have us thinking about writing policies over multiple predicates. However, I think that idea would need to be fleshed out a lot further before we could really reason about them here and if we can avoid that I think we'd be better off for it. |
ITE/11/README.adoc
Outdated
| `expectedAttributes` will not support artifact rules but will instead evaluate | ||
| each rule using the Common Expression Language (CEL). | ||
|
|
||
| If this ITE is applied to the original in-toto layout, the schema for each step |
There was a problem hiding this comment.
nit: I find the "If this ITE is applied" language a bit confusing (especially given the "If instead" construction below).
The conditionality makes it sound like it's up to the user to decide where to use the ITE, when the proposal is actually to apply the ITE to the entire spec and make all of these formulations possible.
If this just the standard way to write ITEs that's cool.
There was a problem hiding this comment.
The conditional is meant to apply to the type of layout. It might help to replace the original in-toto layout with references to v1.0. The reason is, this could easily apply to legacy layout schemas without depending on the layout proposed in #49.
There was a problem hiding this comment.
My biggest feedback for this ITE is that it will be impossible to get consensus on an expression language of choice. We could ofc be strongly opinionated about it, but I think we should repeat what we did with signing envelopes: while we do not mandate any particular one, we strongly recommend DSSE, and support it out-of-the-box. We should be similarly liberal about the choice of policy engine while choosing one by default.
There was a problem hiding this comment.
As I said earlier in a comment, my biggest feedback for this ITE is that it will be impossible to get consensus on an expression language of choice. We could ofc be strongly opinionated about it, but I think we should repeat what we did with signing envelopes: while we do not mandate any particular one, we strongly recommend DSSE, and support it out-of-the-box. We should be similarly liberal about the choice of policy engine while choosing one by default.
However, I am happy to approve this PR as a working draft so that we can iterate and make progress. Sorry for the delay in review, and thanks for all your hard work!
|
I agree, I think it makes sense to generally call for an expression language with a set of properties, and recommend the use of a single one, possibly / likely CEL. |
To properly generalise at the risk of slightly complicating this ITE, there may be need to be an We may also want to discuss the security implications (e.g., computational or memory constraints) of choosing any expression language. |
| ```yaml | ||
| predicateType: string | ||
| expectedProducts: list of artifact rules | ||
| expectedAttributes: list of CEL expressions |
There was a problem hiding this comment.
A raw list of CEL expressions might be a bit limiting. If something fails, it's difficult to present a clear error message of what went wrong. You might want to have a field for error messages. (Similarly, giving each an optional name might make it more readable.)
For example:
expectedAttributes:
- name: builderId
require: "predicate.runDetails.builder.id == 'https://example.com/myBuilder'"
errorMessage: '"Unexpected builder.id: expected \"https://example.com/myBuilder\", got " + predicate.runDetails.builder.id'
- name: repository
require: "predicate.externalParameters.repository == '<expected value>'"
errorMessage: '...'There was a problem hiding this comment.
This is really great! I'm going to work on this and support it in attestation-verifier, but without blocking merging this ITE as draft as it's been an open PR for some time now.
ITE/11/README.adoc
Outdated
| [[backwards-compatibility]] | ||
| == Backwards Compatibility | ||
|
|
||
| This ITE is entirely backwards compatible with both the in-toto v1.0 layout |
There was a problem hiding this comment.
Backwards compatible with what? Is this a legacy client and a new layout? A new client and a legacy layout, etc.? I don't think it is always backwards compatible.
There was a problem hiding this comment.
Clarified this some more to indicate that backwards compatibility isn't a given, we may have situations where a check that ought to have failed actually passes because a legacy client skips attribute checks.
| expectedMaterials: list of artifact rules | ||
| expectedProducts: list of artifact rules |
There was a problem hiding this comment.
Checking for clarification: the in-toto spec for the Steps format specifies these fields as expected_materials and expected_products.
I haven't seen any ITEs or spec updates that change this, but ITE-10 and this ITE-11 (as well as the project at https://github.com/in-toto/attestation-verifier) seem to use this camelCase for these same fields.
Is it the case that:
- The documentation refers to these fields in
camelCase, but that's just for convenience, and actual serialization should remainsnake_case - The layout schema outlines the fields that should exist, but the actual naming/serialization format for the fields is considered implementation-dependent and is not important to standardize
- There is an intent to move to
camelCasefor the standard naming of fields in the layout schema, but the spec just hasn't been updated
We are using our own in-toto implementation for verification and are attempting to adhere to the spec as much as possible/practical, but are having a hard time determining whether there is a correct/canonical serialization format based on these inconsistencies.
There was a problem hiding this comment.
For me personally, it was mostly 1, though it's something to consider and nail down for compatibility / consistency. Happy to change these back to snake case unless there's interest in using this breaking change to also change the serialization format. I don't feel strongly one way or another.
FWIW, I think we should probably reject 2 if it does come up; it's tempting to treat this as an implementation detail but we should aim for maximum compatibility of layouts and corresponding tooling. @jkjell we should coordinate this, if applicable, wrt witness policies too.
ITE/11/README.adoc
Outdated
|
|
||
| ```yaml | ||
| predicateType: string | ||
| expectedProducts: list of artifact rules |
There was a problem hiding this comment.
Is expectedProducts meant to be at this level, or at the step schema level? In the previous section that discusses applying this ITE to the 1.0 schema, expectedProducts is still at the step level, and that is also where it is in the in-toto/attestation-verifier project: https://github.com/in-toto/attestation-verifier/blob/fc1406e6ae8b41696091061712feaee6bfdd263a/verifier/models.go#L40
There was a problem hiding this comment.
Good catch, for the new layout, we were experimenting with having products at the predicate level if we wanted to support multiple predicates per step, but eventually, we moved it back up. Attributes still apply at the predicate level. I'll make this change shortly.
|
Hi @adityasaky, I was wondering if you could comment on the overall state of ITE-10 and ITE-11 and their relations to the in-toto spec. It looks like ITE-10 merged about a year ago (with "Draft" status), and the core of the ITE-11 PR was from mid to late last year as well. The core in-toto spec is at v1.0.0 as of June 2023. What is the expected process/timeline for ITE-10 and ITE-11 to advance and/or graduate from draft status? Additionally, both ITE-10 and ITE-11 propose changes to the layout file schema, so in order for them to be "officially" used, the core spec would need to be updated to reflect them as well. Was just wondering whether there were specific tasks or actions that needed to occur for this to happen that are actively in progress, or if this is somewhat in limbo/holding pattern due to external factors. As context, I am interested in migrating to using the ITE-10/11 version of in-toto verification with attestations, and although it is possible for me to proceed based on the ITEs, ideally it would be nice to be able to be using a release version of in-toto (rather than having to caveat that I'm implementing "v1.0.0 with ITE-10 and ITE-11 proposals as of July 2024"). Thanks! |
|
Hi @nmiyake! The general process is an ITE gets merged as draft, and after it's been implemented and we've had a chance to receive feedback from consumers, we decide whether it can be accepted. At that point, we'd update the spec to incorporate the ITE. Essentially, the more data points we get about the ITE in practice, the faster things can move I think (speaking from my personal perspective). The status of ITE-10 and 11 is we're essentially waiting for feedback on if the new layout schemas make sense for different consumers and their use cases, especially given the magnitude of the changes. We actually just discussed getting ITE-11 merged as a draft soon, @trishankatdatadog will be helping drive that. I hope that once we get this PR merged, and we reconcile any drift in ITE-10/11 with attestation-verifier / other implementations of the ITEs, we'll start hearing (from you and others!) what else needs to go into these ITEs. On a related note: we kicked off a working group to drive in-toto policies onwards today. We would love to have you on those calls, they should be on a public calendar or so somewhere in the near future, I think. cc @trishankatdatadog, @jkjell, @Forrin |
I approved it last year, apparently, and completely forgot. @SantiagoTorres, could we please get a LGTM from you to unblock?
@nmiyake Just ping me on the CNCF Slack and I'll be happy to add you, thanks. |
|
@adityasaky @trishankatdatadog thanks! I've joined the CNCF Slack and pinged there for further information. For context, our organization has been using in-toto to perform verification of deployment artifacts since around late 2021 using the pre-1.0 version. We're in the process of migrating to use attestations so that we can record and verify more information, and as of right now are planning to use a combination of in-toto v1 + ITE-10/ITE-11 proposals. As you can probably tell from some of my comments, in doing so I've found some occasional inconsistencies between different parts of the specs and reference implementations. From the external perspective, it's been hard to tell exactly what the status is for the specs and reference implementations etc., so getting some initial context around that would probably be most helpful for me. Although my primary focus is for our internal use case, I think that having a robust and accurate official spec and reference implementation(s)/libraries would be a benefit to everyone, and I'd be happy to look to contribute work across the spec and any Go/Java implementations, but would want to make sure that I have the proper overall context first (for example sounds like the current Go library may be deprecated in favor of go-witness, which isn't context I knew about prior to some of the comments here). Happy to engage either via a comment thread here or over Slack or email if it makes sense to do so. Thanks! |
Signed-off-by: Aditya Sirish <[email protected]>
|
Hi all. This PR has received plenty of feedback, and I think it's come a long way. I'm gleaning from the conversations that everybody is happy to merge as draft. There's still space to refine things before it's marked as accepted. |
This was previously part of #38. I've broken that one down into separate ITEs so it's easier to reason about the changes proposed.
Prototyping here: https://github.com/adityasaky/in-toto-attestation-verifier